-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding Orbitsoft adapter #1378
Adding Orbitsoft adapter #1378
Conversation
modules/orbitsoftBidAdapter.js
Outdated
let bids = params.bids || []; | ||
|
||
if (bids.length === 0) { | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this conditional is kind of pointless. if bids.length === 0
then the for loop below won't iterate and the function will return anyways
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
// @if NODE_ENV='debug' | ||
utils.logMessage('No param requestUrl'); | ||
// @endif | ||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there are errors creating the bid request, you should return a NO_BID to prebid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed (on line 47)
cpm: 0 | ||
}; | ||
|
||
pbjs._bidsRequested.push(bidderRequest); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't need to mess with Prebid's private vars. This has the ability to affect other tests, plus it's a private var so it could possible it could change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this is done by push method, which not has affect other tests, and this is exactly what is recommended by the official document at http://prebid.org/dev-docs/testing-prebid.html
And this technique is used for most other adapter tests for prebid.js
@snapwich what's the status on this? |
modules/orbitsoftBidAdapter.js
Outdated
callBids: baseAdapter.callBids, | ||
setBidderCode: baseAdapter.setBidderCode, | ||
buildJPTCall: buildJPTCall | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order for your adapter to be alias-able the .constructor
reference on your adapter instances needs to point to the correct constructor function; by returning a new object that reference will be incorrect.
You can solve this with:
return Obect.assign(this, {
callBids: baseAdapter.callBids,
setBidderCode: baseAdapter.setBidderCode,
buildJPTCall: buildJPTCall
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@snapwich Fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, you need to modify it to be the way I described (return this
). You can look at another adapter if you wish to get an example, but the modifications you made look like the old alias pattern. createNew
is no longer needed, but the constructor functions must return a reference to this
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@snapwich Sorry for this. I made changes.
modules/orbitsoftBidAdapter.js
Outdated
}; | ||
|
||
adaptermanager.registerBidAdapter(new OrbitsoftAdapter(), ORBITSOFT_BIDDERCODE); | ||
adaptermanager.aliasBidAdapter('orbitsoft', 'orbitadserving'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why so many aliases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mkendall07 We rebranded recently from Orbitscripts with Orbit Ad Serving product to Orbitsoft.
If too much, I can delete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is a bit confusing to have so many. Typically if the code changes we put the alias in to help with transition but since this is a new adapter I don't think you need them. You should use your current preferred name only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mkendall07 It is fair remark. Deleted it.
for (let i = 0; i < bids.length; i++) { | ||
let bidRequest = bids[i]; | ||
let callbackId = bidRequest.bidId; | ||
let jptCall = buildJPTCall(bidRequest, callbackId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you are hitting the AppNexus endpoint. Is there a reason you cannot use an AppNexus alias instead of creating your own adapter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mkendall07 AppNexus has built-in enpoint URL for bid requesting. Our adapter sends bid request to various endpoint URL where customers install our product. Endpoint URL is defined by the requestUrl parameter and different for diffrent customers.
We also have additional options for requesting bids and ads.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…built * 'master' of https://github.com/prebid/Prebid.js: (46 commits) Serverbid alias (prebid#1560) Add user sync to process for approving adapter PRs (prebid#1457) fix travis build (prebid#1595) Rubicon project improvement/user sync (prebid#1549) Adding Orbitsoft adapter (prebid#1378) Fix renderer test for new validation rule (prebid#1592) Allow SET_TARGETING to be used in AnalyticsAdapter (prebid#1577) Add support for video stream context (prebid#1483) Invalidate bid if matching bid request not found (prebid#1575) allow adapters to set default adserverTargeting for specific bid (prebid#1568) Custom granularity precision should honor 0 if it is passed in closes prebid#1479 (prebid#1591) BaseAdapter for the Prebid 0.x -> 1.x transition (prebid#1494) Add a version to the Criteo adapter (prebid#1573) Allow bundling from node.js or with new gulp task bundle-to-stdout (prebid#1570) Add url.parse option to not decode the whole URL (prebid#1480) Tremor Video Bid Adapter (prebid#1552) Yieldmo bid adapter (prebid#1415) Switch `gulp docs` to build its output using documentation.js (prebid#1545) Increment pre version Prebid 0.28.0 Release ...
* Adding Orbitsoft module * Adding Orbitsoft module (corrected) * Adding Orbitsoft module (correction of remarks) * Adding Orbitsoft module (correction of remarks) * Adding Orbitsoft module (correction to alias-able) * Adding Orbitsoft module (correction to alias-able) * Adding Orbitsoft module (correction to alias-able) * Adding Orbitsoft module (correction to alias-able) * Adding Orbitsoft module (correction to new constructor) * Adding Orbitsoft module (delete unnecessary aliases) * Adding Orbitsoft module (delete unnecessary aliases)
* tag '0.29.0' of https://github.com/prebid/Prebid.js: (29 commits) Prebid 0.29.0 Release Fix for not syncing bidders. (prebid#1598) fix amp example pages (prebid#1597) closes prebid#1298 (prebid#1583) Fixed the broken tests. (prebid#1602) Rubicon Bidder Factory (prebid#1587) Trustx adapter (prebid#1488) Add nurl to markup (prebid#1601) Pass bidRequest to createBid (prebid#1600) Add Kumma adapter (prebid#1512) Serverbid alias (prebid#1560) Add user sync to process for approving adapter PRs (prebid#1457) fix travis build (prebid#1595) Rubicon project improvement/user sync (prebid#1549) Adding Orbitsoft adapter (prebid#1378) Fix renderer test for new validation rule (prebid#1592) Allow SET_TARGETING to be used in AnalyticsAdapter (prebid#1577) Add support for video stream context (prebid#1483) Invalidate bid if matching bid request not found (prebid#1575) allow adapters to set default adserverTargeting for specific bid (prebid#1568) ...
* Adding Orbitsoft module * Adding Orbitsoft module (corrected) * Adding Orbitsoft module (correction of remarks) * Adding Orbitsoft module (correction of remarks) * Adding Orbitsoft module (correction to alias-able) * Adding Orbitsoft module (correction to alias-able) * Adding Orbitsoft module (correction to alias-able) * Adding Orbitsoft module (correction to alias-able) * Adding Orbitsoft module (correction to new constructor) * Adding Orbitsoft module (delete unnecessary aliases) * Adding Orbitsoft module (delete unnecessary aliases)
Type of change
Description of change
New Orbitsoft bidder
Be sure to test the integration with your adserver using the Hello World sample page.
support@orbitsoft.com
Other information